-
-
Notifications
You must be signed in to change notification settings - Fork 415
Function package rework and named function arguments #8112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev/feature
Are you sure you want to change the base?
Conversation
…o feature/named-function-args
|
Why have the builds been failing for months? Do you need assistance? |
APickledWalrus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple early thoughts. Tests are still failing :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be part of StructFunction test IMO
| format: A namespaced key can be formatted as 'namespace:id' or 'id'. It can only contain one ':' to separate the namespace and the id. Only alphanumeric characters, periods, underscores, and dashes can be used. | ||
|
|
||
| # -- Functions -- | ||
| functions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep single quotes around function/parameter names. It improves readability.
| return new NonNullPair<>(word, false); | ||
| } | ||
|
|
||
| public record PluralResult(String updated, boolean plural) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs docs
| * @deprecated Use {@link #isPlural(String)} instead. | ||
| */ | ||
| @Deprecated(forRemoval = true, since = "INSERT VERSION") | ||
| public static NonNullPair<String, Boolean> getEnglishPlural(String word) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just call the new method and convert the result into a NonNullPair.
| * | ||
| * @param <T> The type of the function parameter. | ||
| */ | ||
| @NonExtendable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be a sealed interface?
| /** | ||
| * @return An unmodifiable view of all the parameters that this signature has. | ||
| */ | ||
| @Unmodifiable @NotNull SequencedMap<String, Parameter<?>> parameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use UnmodifiableView annotation instead if docs are correct
| @Internal | ||
| @Experimental | ||
| public interface Signature<T> { | ||
| public interface Signature<T> extends Documentable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not force Documentable here. It should be optional.
| /** | ||
| * @return An unmodifiable view of all the parameters that this signature has. | ||
| */ | ||
| @Unmodifiable @NotNull SequencedMap<String, Parameter<?>> parameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand why this needs to be a map. Don't parameters store their names?
Is it also critical that it be sequenced? If we can reference function parameters by name, does ordering really matter?
Problem
Skript should add named function arguments, similar to languages like Kotlin. This increases the clarity of which argument is passed.
Example:
Also, the function package code is a mess.
Solution
Named function arguments
Named function arguments act as you would expect. They are always usable, except for when mixing named and unnamed arguments. To avoid assigning the incorrect value to a parameter, mixing named/unnamed is only allowed when the named arguments are in the same order as the parameters of the function. An example.
Lists are also supported.
Rework changes
Function references
Function references have had a major rework. These changes are extensive and happen predominantly in
SkriptParser.To try and improve organization, all function parsing code has been moved to theFunctionParsersubclass. To accommodate these changes ,njol.FunctionReferencehas been deprecated and has been replaced byskript.FunctionReference.Function references are now determined inside the parser by using existing function signatures, which allows it to automatically select the correct function reference based on the argument types (fixing #8106). For every signature, it will attempt to parse the given arguments to the types belonging to the signature. If this is successful, it will continue with all other signatures. Then, if only one signature is valid, an exact match has been found. If there are multiple matches, an error will be printed.
The integration with
SkriptParseralso removes the need to fetch the signature of functions at runtime, as these can be stored directly inside the reference, which improves performance and reduces complexity.Function arguments
Since Skript doesn't support the format to achieve named function arguments, a new function reference argument parser has been written. On receiving the input string, this parser correctly separates the arguments into arguments, and also determines whether these arguments are named or not. An example output is as follows.
Function execution
Since position is no longer relevant, all function execution is now handled via the abstract
Function#execute(FunctionEvent, FunctionArguments)method. Instead of a 2D object array, functions now have access toFunctionArguments, which is essentially a glorified map. This map holds all the passed function arguments by name. The old execution method has been deprecated.Parameters
njol.Parameterhas been deprecated and has been replaced by the following classes, both of which implement the newskript.Parameterinterface.skript.ScriptParameter: for parameters that are parsed from scripts. This class has an additional default value attribute, as well as the attributes provided byskript.Parameter. This default value is used in e.g.x: int = 3;skript.DefaultParameter: for parameters forDefaultFunctions. This class has no additional attributes other than those provided byskript.Parameter;These new parameters use classes instead of classinfos for their types. When a parameter accepts multiple values, its type is e.g.
Integer[]orString[]. When it accepts a single value, it is e.g.IntegerorString. Parameters also have a set of modifiers, which dictate the behaviour of the parameter. TheModifierinterface details the two currently used parameters:OPTIONAL: for function parameters that are optional,KEYED: for function parameters that accept keyed values.Furthermore, parameters are now stored as a
LinkedHashMap, which encourages the intended use for accessing function arguments. Using names and accessing the first argument are easy, while getting a parameter by its position is now more involved. Due to Skript still supporting Java 17,SequencedMapcannot be used as an abstraction ofLinkedHashMapyet.Parameter parsing
Script parameter parsing has been relocated from the (soon-to-be-extinct)
Functionsclass. Parsing a single parameter has been moved toScriptParameter, and parsing the entire signature of a script function has been moved to the new subclassStructFunction.FunctionParser.Adds -> as a function return prefix
Looks nicer than the others :)
Testing Completed
Supporting Information
Breaking changes
SkriptParser#parseFunction(Class[])has been entirely replaced withSkriptParser#parseFunctionReference()Function#execute(FunctionEvent, FunctionArguments)must be implemented by anyone extendingFunctiondirectlySignature#getParameters()andSignature#getParameter(int)have had their return type changed toskript.Parameterinstead ofnjol.ParameterExprFunctionCallandEffFunctionCall's constructors have changed from acceptingnjol.FunctionReferenceto now acceptingskript.FunctionReference.JavaFunction#execute, the implementation oflocationhas been updated to avoid the world being null. (as described in Java function rework #7969)njol.Parameterandnjol.Signaturehave been changed to be private, with methods being added for access.ScriptFunction#setReturnValue(deprecated since 2.9)SkriptParser#wildcard(deprecated since 2.8)SkriptParser#listSplitPattern(deprecated since 2.7)Completes: #7987, #8106
Related: none